MDEV-39520: Add MySQL 8.0 extended syntax for REGEXP_INSTR#5035
MDEV-39520: Add MySQL 8.0 extended syntax for REGEXP_INSTR#5035MohamedM216 wants to merge 1 commit into
REGEXP_INSTR#5035Conversation
MohamedM216
force-pushed
the
mysql8-extended-regexp
branch
2 times, most recently
from
d2ac796 to
ef7d499
Compare
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
First of all: this is implementing just one sub-task of what https://jira.mariadb.org/browse/MDEV-39106 calls for. Either do all of it, as scoped in the jira, in a single go. Or create Jira sub-task(s) into the above jira and target these instead.
There's also some suggestions below.
MohamedM216
changed the title
REGEXP_INSTRREGEXP_INSTR
MohamedM216
force-pushed
the
mysql8-extended-regexp
branch
2 times, most recently
from
d0e479a to
2634104
Compare
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Please fix the failing buildbot tests. Some improvement suggestions below too.
MohamedM216
force-pushed
the
mysql8-extended-regexp
branch
from
2634104 to
1518554
Compare
|
The current failing tests aren't related to my patch and they pass locally. |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Please stand by for the final review.
sanja-byelkin
left a comment
sanja-byelkin
left a comment
There was a problem hiding this comment.
Found implementation problem should be fixed. Also performance and functionality test should be made after code review done
|
|
||
| re.init(cmp_collation.collation, 0); | ||
|
|
||
| if (arg_count > 5 && args[5]->const_item()) |
There was a problem hiding this comment.
const_item is not basic constant. prepare place holder (parameter '?') is a constant for example but changes, some other cases also possible if I remember correctly. So tests with changing parameter of prepared statement should be added.
There was a problem hiding this comment.
A test for this case is added now at the end of the test file.
| { | ||
| char mt_buf[64]; | ||
| String mt_tmp(mt_buf, sizeof(mt_buf), &my_charset_latin1); | ||
| String *match_type_str= args[5]->val_str(&mt_tmp); |
There was a problem hiding this comment.
If It was constant and flags was calculated on fix field what sens to repeat here again and again the same exercise?
| if ((null_value= args[5]->null_value)) | ||
| return 0; | ||
|
|
||
| if (!args[5]->const_item()) |
There was a problem hiding this comment.
And here we are suppose to use old value but above we get string value of the Item what the sens in above evaluation?
There was a problem hiding this comment.
Also constant flag can change try this with tables of 1 row length and it will fail (please add such test also). i.e. if we cached values on fix_field w should not detect by call of const_item during execution bacouse we have make_const calls, we have substitution and maybe I forgot about something else. So please use something else (maybe just boolean flag)
There was a problem hiding this comment.
Also constant flag can change try this with tables of 1 row length and it will fail
You're right, it fails, but using the boolean trick to store const_item()'s value then reuse it fixes this bug.
The test is found at the end of the test file.
| } | ||
|
|
||
| char subject_buf[MAX_FIELD_WIDTH]; | ||
| String subject_tmp(subject_buf, sizeof(subject_buf), &my_charset_bin); |
There was a problem hiding this comment.
I am not specialist in regexp but I feel it can be not so efficient as previous code. So testing of performance should be done. @gkodinov could you organise one. Also it is new feature which rewrite rexexp so after code review it have to go to a tester review as we do with all features.
Extend REGEXP_INSTR to accept the full MySQL 8.0 signature:
REGEXP_INSTR(subject, pattern
[, pos [, occurrence [, return_option
[, match_type]]]])
Previously only 2 arguments were accepted. Changes:
- Switch Create_func_regexp_instr from Create_func_arg2 to
Create_native_func to allow 2-6 arguments.
- Add parse_match_type_flags() to Regexp_processor_pcre, which parses
the match_type flags (c/i/m/n/u) and overwrites m_library_flags
with the fully-resolved PCRE2 flag word.
- Call parse_match_type_flags() in fix_length_and_dec() before fix_owner()
when match_type is constant, so the pattern is compiled with the
correct flags. For constant patterns fix_owner() sets m_is_const=true,
making recompile() a permanent no-op.
- For non-constant match_type, resolve flags per-row in val_int()
and call compile() directly to bypass the recompile() no-op guard.
- Add MTR test: regexp_instr_mysql8.test
MohamedM216
force-pushed
the
mysql8-extended-regexp
branch
from
1518554 to
96f67b0
Compare
|
Thanks @sanja-byelkin for the detailed feedback! |
3 participants
Reference Issue
MDEV-39520
Description
This PR is one of a series of PRs that are supposed to add MySQL 8.0-compatible extended syntax for all REGEXP functions
Test
regexp_instr_mysql8.test